Support Ledger clear signing #1603
Conversation
| # Ledger constants | ||
| STARKNET_CLA = 0x5A | ||
| EIP_2645_PURPOSE = 0x80000A55 | ||
| EIP_2645_PATH_LENGTH = 6 | ||
| PUBLIC_KEY_RESPONSE_LENGTH = 65 | ||
| SIGNATURE_RESPONSE_LENGTH = 65 | ||
| VERSION_RESPONSE_LENGTH = 3 | ||
|
|
There was a problem hiding this comment.
note: These variables aren't used across multiple files so let's keep them in the dedicated (ledger) one.
| recipient_address = ( | ||
| 0x1323CACBC02B4AAED9BB6B24D121FB712D8946376040990F2F2FA0DCF17BB5B | ||
| ) | ||
| recipient_address = 0x123 |
There was a problem hiding this comment.
note: 0x123 is better as an example address.
| @@ -2,10 +2,13 @@ | |||
| "version": 1, | |||
| "rules": [ | |||
| { | |||
| "text": "Confirm Hash to sign", | |||
| "conditions": [], | |||
| "text": "Blind igning", | |||
There was a problem hiding this comment.
note: It's not a typo - for some reason that's how ledger displays the text
| @pytest.mark.parametrize( | ||
| "transaction", | ||
| [ | ||
| InvokeV3( | ||
| version=3, | ||
| signature=[], | ||
| nonce=1, | ||
| resource_bounds=MAX_RESOURCE_BOUNDS_SEPOLIA, | ||
| calldata=[ | ||
| 1, | ||
| 2009894490435840142178314390393166646092438090257831307886760648929397478285, | ||
| 232670485425082704932579856502088130646006032362877466777181098476241604910, | ||
| 3, | ||
| 0x123, | ||
| 100, | ||
| 0, | ||
| ], | ||
| sender_address=0x123, | ||
| ), | ||
| DeployAccountV3( | ||
| class_hash=0x123, | ||
| contract_address_salt=0x123, | ||
| constructor_calldata=[1, 2, 3], | ||
| version=3, | ||
| signature=[], | ||
| nonce=0, | ||
| resource_bounds=MAX_RESOURCE_BOUNDS_SEPOLIA, | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
note: We can't use mocks for clear singing as under the hood we're converting values from tx fields to bytes, and we would get TypeError: can't concat Mock to bytes.
|
|
||
| def sign_transaction(self, transaction: AccountTransaction) -> List[int]: | ||
| if self.signing_mode == LedgerSigningMode.CLEAR: | ||
| if isinstance(transaction, DeclareV3): | ||
| raise ValueError("DeclareV3 signing is not supported bye LedgerSigner") |
There was a problem hiding this comment.
Do you know why declare is not supported?
There was a problem hiding this comment.
Not really, haven't digged into the specific reason but maybe it's related with device hardware limits (considering that declare tx includes contract class) or security?
| if application_name == "LedgerW": | ||
| application_bytes = ( | ||
| (43).to_bytes(1, byteorder="big") | ||
| + (206).to_bytes(1, byteorder="big") | ||
| + (231).to_bytes(1, byteorder="big") | ||
| + (219).to_bytes(1, byteorder="big") | ||
| ) | ||
| else: | ||
| application_bytes = _string_to_4byte_hash(application_name) |
There was a problem hiding this comment.
Could you explain this logic?
There was a problem hiding this comment.
Seems like these four integers are predefined for LedgerW and in other cases we just generate the representation.
I referred to https://github.com/starknet-io/starknet.js/blob/050616221a58545d79eece7040b4abc205c220e0/src/signer/ledgerSigner221.ts#L644
| if response is None: | ||
| raise ValueError("No response received from Ledger device.") |
There was a problem hiding this comment.
Shouldn't it be checked inside the loop?
There was a problem hiding this comment.
No,apdu_exchange will always either raise an error or return response, so we mostly want to check if the loop was run.
| response = self.app.client.apdu_exchange( | ||
| ins=3, | ||
| data=bytes(calldatas[0]), | ||
| p1=6, |
There was a problem hiding this comment.
Why is it 6 not 5?
https://github.com/LedgerHQ/app-starknet/blob/develop/docs/apdu.md#new-call
There was a problem hiding this comment.
I think it's a typo - look that p1 is 5 here. Will add an issue in their repo for that.
| if len(calldatas) > 1: | ||
| for part in calldatas[1:]: | ||
| response = self.app.client.apdu_exchange( | ||
| ins=3, p1=6, p2=1, data=part |
There was a problem hiding this comment.
Shouldn't the last call be with p2=2?
https://github.com/LedgerHQ/app-starknet/blob/develop/docs/apdu.md#end-of-calldata-for-the-current-call
Closes #1473
Introduced changes
Add support for clear singing in
LedgerSigner. Now we're getting displayed tx details before approving/rejecting it.LedgerSigningModeclear_sign_example.mov